Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Sep 25, 2025

What do these changes do?

Follows up on PR #8405
This continues the migration of aiohttp app keys from plain strings to type-safe web.AppKey.

image image

SEE https://docs.aiohttp.org/en/stable/web_reference.html#aiohttp.web.AppKey

Changes

Before

from typing import Final

APP_MY_KEY: Final[str] = f"{__name__}.APP_MY_KEY"
app[APP_MY_KEY] = value
data = request.app[APP_MY_KEY]

After

from typing import Final
from aiohttp import web

CONSTNAME_APPKEY: Final = web.AppKey("CONSTNAME", SomeSpecificType)
app[CONSTNAME_APPKEY] = value
data = request.app[CONSTNAME_APPKEY]  # type-safe
# no need to cast anymore — mypy infers the correct type

NOTE: this can be done using refactor-aiohttp-appkey.prompt.md


Additions

  • Introduced application_keys.py (or $(domain)/_application_keys.py)
  • Added refactor-aiohttp-appkey.prompt.md to streamline future refactors

Related issue/s

How to test

in place

Dev-ops

None

@pcrespov pcrespov self-assigned this Sep 25, 2025
@pcrespov pcrespov added t:maintenance Some planned maintenance work a:webserver webserver's codebase. Assigning the area is particularly useful for bugs labels Sep 25, 2025
@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 94.76923% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.85%. Comparing base (1dbb64b) to head (2d612cb).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8424      +/-   ##
==========================================
- Coverage   89.01%   87.85%   -1.17%     
==========================================
  Files        1753     1957     +204     
  Lines       68638    76126    +7488     
  Branches      837     1342     +505     
==========================================
+ Hits        61098    66877    +5779     
- Misses       7331     8845    +1514     
- Partials      209      404     +195     
Flag Coverage Δ
integrationtests 64.10% <72.93%> (+0.73%) ⬆️
unittests 86.51% <94.15%> (-1.53%) ⬇️
Components Coverage Δ
pkg_aws_library 93.59% <ø> (ø)
pkg_celery_library 83.41% <ø> (ø)
pkg_dask_task_models_library 79.33% <ø> (ø)
pkg_models_library 93.08% <ø> (ø)
pkg_notifications_library 85.20% <ø> (ø)
pkg_postgres_database 87.95% <ø> (ø)
pkg_service_integration 70.19% <ø> (ø)
pkg_service_library 72.55% <59.09%> (∅)
pkg_settings_library 90.19% <ø> (ø)
pkg_simcore_sdk 84.93% <ø> (+19.65%) ⬆️
agent 93.53% <ø> (ø)
api_server 91.94% <ø> (ø)
autoscaling 95.74% <ø> (ø)
catalog 92.36% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.82% <ø> (-0.56%) ⬇️
datcore_adapter 97.94% <ø> (ø)
director 75.81% <ø> (ø)
director_v2 90.84% <ø> (-0.10%) ⬇️
dynamic_scheduler 96.30% <ø> (ø)
dynamic_sidecar 90.43% <ø> (ø)
efs_guardian 89.62% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.62% <ø> (ø)
resource_usage_tracker 92.18% <ø> (+0.05%) ⬆️
storage 86.65% <ø> (ø)
webclient ∅ <ø> (∅)
webserver 87.86% <97.35%> (+0.08%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dbb64b...2d612cb. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mergify
Copy link
Contributor

mergify bot commented Sep 25, 2025

🧪 CI Insights

Here's what we observed from your CI run for 2d612cb.

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on master Retries 🔍 CI Insights 📄 Logs
CI integration-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View
system-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View
unit-tests Base branch is broken, but retries were needed. Could be early signs of flakiness 👀 Broken 1 View View

@pcrespov pcrespov changed the title Mai/more app keys ♻️ Refactor: migrate more aiohttp app keys to type-safe web.AppKey Sep 26, 2025
@pcrespov pcrespov marked this pull request as ready for review September 26, 2025 08:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request continues the migration of aiohttp app keys from plain string constants to type-safe web.AppKey objects, building on the work started in PR #8405. This refactoring improves type safety and eliminates the need for manual type casting when accessing application state.

Key changes include:

  • Converting string-based app key constants to web.AppKey objects with proper typing
  • Updating all usage patterns throughout the codebase to use the new type-safe keys
  • Creating centralized application_keys.py files to organize these definitions

Reviewed Changes

Copilot reviewed 112 out of 113 changed files in this pull request and generated 3 comments.

File Description
services/web/server/src/simcore_service_webserver/application_keys.py New centralized file defining type-safe application keys
Various plugin and service files Updated to import and use new APP_SETTINGS_APPKEY instead of APP_SETTINGS_KEY
Multiple test files Updated to use new type-safe app keys for testing
Various constant and key definition files Migrated string-based keys to web.AppKey objects

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@pcrespov pcrespov enabled auto-merge (squash) September 26, 2025 09:57
@pcrespov pcrespov added this to the Cheops milestone Sep 26, 2025
@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Sep 26, 2025
@pcrespov
Copy link
Member Author

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Sep 26, 2025

queue

🛑 Configuration not compatible with a branch protection setting

The branch protection setting Require branches to be up to date before merging is not compatible with max_parallel_checks>1, queue_conditions != merge_conditions and must be unset.

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@sonarqubecloud
Copy link

@pcrespov pcrespov merged commit 58f9862 into ITISFoundation:master Sep 26, 2025
145 of 148 checks passed
@pcrespov pcrespov deleted the mai/more-app-keys branch September 29, 2025 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:webserver webserver's codebase. Assigning the area is particularly useful for bugs t:maintenance Some planned maintenance work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants